Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do a hacky workaround directly to the http-server package to fix… #569

Merged
merged 3 commits into from
Nov 10, 2019

Conversation

Xmader
Copy link
Contributor

@Xmader Xmader commented Oct 23, 2019

Please ensure that your pull request fulfills these requirements:

  • The pull request is being made against the master branch
  • Tests for the changes have been added (for bug fixes / features)

What is the purpose of this pull request? (bug fix, enhancement, new feature,...)

Fixes #525

What changes did you make?

do a hacky workaround directly into the http-server package
(patch to https://github.com/jfhbrook/node-ecstatic/blob/master/lib/ecstatic.js#L20)

My solution is so hacky. If it has some side effects, please help.
(If there is another function called decodePathname also, it will be affected.)

@thornjad
Copy link
Member

How does this hack work? Does this matter on non-NT OS's? Could we make tests to ensure this continues to work?

@Xmader
Copy link
Contributor Author

Xmader commented Oct 24, 2019

How does this hack work? Does this matter on non-NT OS's?

writing in progress #569 (comment)

Could we make tests to ensure this continues to work?

I think the tests is already covered it.

If this patch is not applied, or not working, the tests would fail with many errors.

  • the When http-server is listening on 8080 when requesting / test would report Cannot read property 'statusCode' of undefined, as well as the tests regards to compression
  • the When http-server is listening on 8080 and options include custom set http-headers test would report Cannot read property 'headers' of undefined
  • the When http-server is listening on 8080 when robots options is activated test would report Error: Invalid URI "http:///". This can be caused by a crappy redirection. , straight forward and pointful
    It is the same error that ERR_INVALID_REDIRECT when running http-server #525 reported.

If this patch is applied, no error reported.

By the way, the test system can't work on Node 12 unless I upgrade the vows package from 0.7.0 to the latest version (0.8.3) . (but with vows 0.8.3, the servers won't stop automatically after testing)

@thornjad
Copy link
Member

thornjad commented Nov 4, 2019

writing in progress

Do you have an update? This looks to be hacking into path itself, is that right?

@Xmader
Copy link
Contributor Author

Xmader commented Nov 4, 2019

yes, path.normalize

@Xmader
Copy link
Contributor Author

Xmader commented Nov 4, 2019

On Linux (or other Unix-like operating systems), path.normalize("/path//to/file") returns "/path/to/file"
But on Windows, path.normalize("/path//to/file") returns "\path\to\file", it is not a valid path component in URL. (It should be separated by /)

This patch forces the path.normalize function to act like it does on *nix, in this specific call stack.

This doesn't matter on non-NT OS's.

@thornjad
Copy link
Member

thornjad commented Nov 6, 2019

Well really this doesn't look too bad, even if super hacky. Still runs for me on Mac, and tests pass. Well tests fail on Node 12 but I think that's a separate problem. I don't have access to a Windows machine to try this out for sure on there, though.

@Xmader
Copy link
Contributor Author

Xmader commented Nov 6, 2019

The problem on Node 12 is related to the vows test framework.
When I upgrade vows to the latest version (0.8.3), it is solved.

@thornjad
Copy link
Member

thornjad commented Nov 7, 2019

Oh duh you said that was an issue! When I upgrade to vows 0.8.3, however, vows seems to hang after all the tests pass, before reporting success. Are you getting the same?

@Xmader
Copy link
Contributor Author

Xmader commented Nov 7, 2019 via email

@Xmader Xmader mentioned this pull request Nov 7, 2019
2 tasks
@thornjad thornjad added this to the v0.12.0 milestone Nov 9, 2019
@thornjad
Copy link
Member

thornjad commented Nov 9, 2019

@Xmader could you merge master back into this pr? Tests should pass now!

@Xmader
Copy link
Contributor Author

Xmader commented Nov 9, 2019

@Xmader could you merge master back into this pr? Tests should pass now!

no problem

Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As hacky as this indeed is, I think it makes enough sense and appears to work!

@thornjad thornjad changed the title do a hacky workaround directly to the http-server package to fix #525 do a hacky workaround directly to the http-server package to fix… Nov 10, 2019
@thornjad thornjad merged commit c1ea830 into http-party:master Nov 10, 2019
@zbynek zbynek mentioned this pull request Jul 3, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERR_INVALID_REDIRECT when running http-server
2 participants